Skip to content

feat: write our own filesystem tools#362

Closed
shadowfax92 wants to merge 2 commits intomainfrom
feat/remove-pi-mono-depdency
Closed

feat: write our own filesystem tools#362
shadowfax92 wants to merge 2 commits intomainfrom
feat/remove-pi-mono-depdency

Conversation

@shadowfax92
Copy link
Contributor

No description provided.

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Feb 25, 2026

Greptile Summary

Replaced external @mariozechner/pi-agent-core and @mariozechner/pi-coding-agent dependencies with custom filesystem tools implementation.

Major Changes:

  • Implemented 7 filesystem tools: read, write, edit, bash, find, grep, ls
  • Added path validation utilities with symlink resolution
  • Comprehensive test coverage for all tools
  • Cross-platform support (Unix and Windows)
  • Features include: image support, line ending preservation, BOM handling, output truncation

Critical Issue Found:

  • Path traversal vulnerability in path-utils.ts allowing sibling directory access (lines 27, 40, 46)

Confidence Score: 2/5

  • This PR contains a critical path traversal vulnerability that must be fixed before merging
  • Score reflects one critical security issue in path-utils.ts where startsWith checks don't validate path separators, allowing sibling directory access. While the implementation is otherwise well-architected with comprehensive tests, this vulnerability could allow unauthorized filesystem access outside the sandbox.
  • apps/server/src/tools/filesystem/path-utils.ts requires immediate attention for the path traversal fix at lines 27, 40, and 46

Important Files Changed

Filename Overview
apps/server/src/tools/filesystem/path-utils.ts Path traversal vulnerability in startsWith checks (lines 27, 40, 46) - allows sibling directory access
apps/server/src/tools/filesystem/build-toolset.ts Clean toolset builder with proper error handling and metrics logging
apps/server/src/tools/filesystem/read.ts Robust file reading with image support, pagination, and truncation
apps/server/src/tools/filesystem/write.ts Simple write implementation with automatic directory creation
apps/server/src/tools/filesystem/edit.ts Sophisticated edit tool with line ending detection, BOM preservation, and exact matching
apps/server/tests/tools/filesystem/path-utils.test.ts Path validation tests - missing sibling directory test case for the vulnerability

Last reviewed commit: f8ecc87

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

22 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

const parent = resolve(resolved, '..')
try {
const realParent = await realpath(parent)
if (!realParent.startsWith(realCwd) && realParent !== realCwd) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Path traversal vulnerability: startsWith check doesn't validate path separator, allowing access to sibling directories with matching prefixes.

Example attack:

  • cwd: /tmp/safe
  • path: ../safe-evil/malicious.txt
  • Result: /tmp/safe-evil/malicious.txt.startsWith(/tmp/safe) → true

Fix: import sep from node:path and check for separator after prefix:

Suggested change
if (!realParent.startsWith(realCwd) && realParent !== realCwd) {
if (!realParent.startsWith(realCwd + sep) && realParent !== realCwd) {

Also apply same fix to lines 40 and 46.

Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/server/src/tools/filesystem/path-utils.ts
Line: 27

Comment:
Path traversal vulnerability: `startsWith` check doesn't validate path separator, allowing access to sibling directories with matching prefixes.

Example attack:
- cwd: `/tmp/safe`
- path: `../safe-evil/malicious.txt`
- Result: `/tmp/safe-evil/malicious.txt`.startsWith(`/tmp/safe`) → `true` ✗

Fix: import `sep` from `node:path` and check for separator after prefix:
```suggestion
        if (!realParent.startsWith(realCwd + sep) && realParent !== realCwd) {
```

Also apply same fix to lines 40 and 46.

How can I resolve this? If you propose a fix, please make it concise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant